fix: add legacy messages back to codec #2909
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple files, focusing on enhancements to message definitions and functionality for cross-chain transactions. Key changes include the addition of new message types, modifications to existing message structures, and updates to the codec registration for backward compatibility. The changelog reflects the introduction of features such as stateful precompiled contracts and improvements to Bitcoin chain support. Additionally, there are refactoring efforts to clean up code and expand testing coverage for newly added functionalities. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2909 +/- ##
===========================================
- Coverage 66.05% 65.80% -0.25%
===========================================
Files 397 398 +1
Lines 22313 22397 +84
===========================================
Hits 14739 14739
- Misses 6806 6890 +84
Partials 768 768
|
|
I've set up a basic Athens full node from a v19 snapshot, and ran it to the v20 upgrade. At the v20 upgrade height, I used a binary built from https://github.com/zeta-chain/node/tree/v20-restore-old-messages. That branch was based on The v20 flavor of the binary I produced runs, though the node only has blocks from 6711001 onward, which is after the v19 upgrade and presumably is missing knowledge of the blocks which contains the messages we need to test. If needed, we can run this v20 binary with a copy of an archive node's data. Were there any messages present in v19 but not v20 that could be used to test? |
The test would be the same as on the initial issue #2835 This PR should solve the error encountered during the query but it is on mainnnet. If needed I can look into having an example to test with Athens3 |
|
The same query provided in the issue also fails when pointed at an Athens testnet archive node. It returns this response: |
|
This does seem to fix |
Ok, wanted to check with the initial query of the issue first. I checked the previous breaking changes, we should not have other message used frequently that should have this compatibility issue |
lumtis
requested review from
brewmaster012,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (5)
typescript/zetachain/zetacore/crosschain/index.d.ts (1)
8-8: Approve the addition of legacy message exports.The introduction of the
legacy_msgs_pbmodule export is consistent with the PR objectives and enhances backward compatibility for pre-v19 transactions. This change appropriately addresses the issues outlined in #2835.Consider organizing the exports alphabetically for improved readability and maintainability. Apply the following diff to achieve this:
export * from "./cross_chain_tx_pb"; export * from "./events_pb"; export * from "./gas_price_pb"; export * from "./genesis_pb"; export * from "./inbound_hash_to_cctx_pb"; export * from "./inbound_tracker_pb"; export * from "./last_block_height_pb"; -export * from "./legacy_msgs_pb"; export * from "./outbound_tracker_pb"; +export * from "./legacy_msgs_pb"; export * from "./query_pb"; export * from "./rate_limiter_flags_pb"; export * from "./tx_pb";x/crosschain/types/codec.go (2)
22-29: Approve changes with a minor suggestion for improved clarity.The addition of these message types for backward compatibility aligns well with the PR objectives. It addresses the issues related to querying pre-v19 transactions as outlined in the linked issue #2835.
To enhance clarity and maintainability, consider adding a version number to the comment:
- // unused messages defined for backward compatibility + // unused messages defined for backward compatibility with pre-v19 transactionsThis modification would provide future developers with more context about when these message types were relevant.
45-52: Approve changes with a suggestion for improved code organization.The additions to the
RegisterInterfacesfunction correctly mirror the changes made in theRegisterCodecfunction, maintaining consistency and addressing the PR objectives.To enhance code organization and readability, consider refactoring the backward compatibility messages into a separate slice:
func RegisterInterfaces(registry cdctypes.InterfaceRegistry) { + backwardCompatibilityMsgs := []sdk.Msg{ + &MsgAddToInTxTracker{}, + &MsgAddToOutTxTracker{}, + &MsgRemoveFromOutTxTracker{}, + &MsgVoteOnObservedOutboundTx{}, + &MsgVoteOnObservedInboundTx{}, + &MsgGasPriceVoter{}, + } + registry.RegisterImplementations((*sdk.Msg)(nil), &MsgAddOutboundTracker{}, &MsgAddInboundTracker{}, &MsgRemoveOutboundTracker{}, &MsgVoteGasPrice{}, &MsgVoteOutbound{}, &MsgVoteInbound{}, &MsgWhitelistERC20{}, &MsgMigrateTssFunds{}, &MsgUpdateTssAddress{}, &MsgAbortStuckCCTX{}, &MsgUpdateRateLimiterFlags{}, - // unused messages defined for backward compatibility - &MsgAddToInTxTracker{}, - &MsgAddToOutTxTracker{}, - &MsgRemoveFromOutTxTracker{}, - &MsgVoteOnObservedOutboundTx{}, - &MsgVoteOnObservedInboundTx{}, - &MsgGasPriceVoter{}, + // unused messages defined for backward compatibility with pre-v19 transactions + backwardCompatibilityMsgs..., ) msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc) }This refactoring improves code organization, reduces duplication, and makes it easier to manage these backward compatibility messages in the future.
docs/spec/crosschain/messages.md (1)
Line range hint
159-163: Approval: New fields added toMsgVoteInboundwith suggestion for documentation.The addition of
asset,event_index,protocol_contract_version,revert_options, andcall_optionsfields to theMsgVoteInboundmessage is consistent with the PR objectives. The field numbering is correct, which should maintain compatibility with existing implementations.These new fields appear to provide additional context and options for inbound transactions, potentially enhancing the system's functionality. To ensure clarity for developers and maintainers:
Consider updating the message description to briefly explain the purpose and usage of these new fields. This will help maintain clear and up-to-date documentation.
🧰 Tools
🪛 Markdownlint
63-63: Column: 1
Hard tabs(MD010, no-hard-tabs)
64-64: Column: 1
Hard tabs(MD010, no-hard-tabs)
65-65: Column: 1
Hard tabs(MD010, no-hard-tabs)
66-66: Column: 1
Hard tabs(MD010, no-hard-tabs)
proto/zetachain/zetacore/crosschain/legacy_msgs.proto (1)
13-15: Mark legacy messages as deprecatedSince these messages are intended for legacy codec compatibility, it's advisable to mark them as deprecated to prevent their use in new code and to signal their eventual phase-out.
Add the
deprecated = trueoption to each legacy message:message MsgAddToOutTxTracker { + option deprecated = true; string creator = 1; // ... } message MsgAddToInTxTracker { + option deprecated = true; string creator = 1; // ... } message MsgRemoveFromOutTxTracker { + option deprecated = true; string creator = 1; // ... } message MsgVoteOnObservedOutboundTx { + option deprecated = true; string creator = 1; // ... } message MsgVoteOnObservedInboundTx { + option deprecated = true; string creator = 1; // ... } message MsgGasPriceVoter { + option deprecated = true; string creator = 1; // ... }This clearly communicates to developers that these messages are deprecated and should not be utilized in future implementations.
Also applies to: 25-27, 37-39, 45-47, 69-71, 94-96
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
typescript/zetachain/zetacore/crosschain/legacy_msgs_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/crosschain/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/legacy_msgs.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (7)
- changelog.md (1 hunks)
- docs/spec/crosschain/messages.md (1 hunks)
- proto/zetachain/zetacore/crosschain/legacy_msgs.proto (1 hunks)
- proto/zetachain/zetacore/crosschain/tx.proto (1 hunks)
- typescript/zetachain/zetacore/crosschain/index.d.ts (1 hunks)
- x/crosschain/types/codec.go (2 hunks)
- x/crosschain/types/legacy_msgs.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
proto/zetachain/zetacore/crosschain/legacy_msgs.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.proto/zetachain/zetacore/crosschain/tx.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/crosschain/types/codec.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/legacy_msgs.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 Markdownlint
docs/spec/crosschain/messages.md
66-66: Column: 1
Hard tabs(MD010, no-hard-tabs)
🪛 buf
proto/zetachain/zetacore/crosschain/legacy_msgs.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 GitHub Check: codecov/patch
x/crosschain/types/legacy_msgs.go
[warning] 13-14: x/crosschain/types/legacy_msgs.go#L13-L14
Added lines #L13 - L14 were not covered by tests
[warning] 17-18: x/crosschain/types/legacy_msgs.go#L17-L18
Added lines #L17 - L18 were not covered by tests
[warning] 21-24: x/crosschain/types/legacy_msgs.go#L21-L24
Added lines #L21 - L24 were not covered by tests
[warning] 26-26: x/crosschain/types/legacy_msgs.go#L26
Added line #L26 was not covered by tests
[warning] 29-31: x/crosschain/types/legacy_msgs.go#L29-L31
Added lines #L29 - L31 were not covered by tests
[warning] 34-35: x/crosschain/types/legacy_msgs.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 42-43: x/crosschain/types/legacy_msgs.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 46-47: x/crosschain/types/legacy_msgs.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 50-53: x/crosschain/types/legacy_msgs.go#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: x/crosschain/types/legacy_msgs.go#L55
Added line #L55 was not covered by tests
[warning] 58-60: x/crosschain/types/legacy_msgs.go#L58-L60
Added lines #L58 - L60 were not covered by tests
[warning] 63-64: x/crosschain/types/legacy_msgs.go#L63-L64
Added lines #L63 - L64 were not covered by tests
[warning] 71-72: x/crosschain/types/legacy_msgs.go#L71-L72
Added lines #L71 - L72 were not covered by tests
[warning] 75-76: x/crosschain/types/legacy_msgs.go#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 79-82: x/crosschain/types/legacy_msgs.go#L79-L82
Added lines #L79 - L82 were not covered by tests
[warning] 84-84: x/crosschain/types/legacy_msgs.go#L84
Added line #L84 was not covered by tests
[warning] 87-89: x/crosschain/types/legacy_msgs.go#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 92-93: x/crosschain/types/legacy_msgs.go#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 100-101: x/crosschain/types/legacy_msgs.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 104-105: x/crosschain/types/legacy_msgs.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 108-111: x/crosschain/types/legacy_msgs.go#L108-L111
Added lines #L108 - L111 were not covered by tests
[warning] 113-113: x/crosschain/types/legacy_msgs.go#L113
Added line #L113 was not covered by tests
[warning] 116-118: x/crosschain/types/legacy_msgs.go#L116-L118
Added lines #L116 - L118 were not covered by tests
[warning] 121-122: x/crosschain/types/legacy_msgs.go#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 129-130: x/crosschain/types/legacy_msgs.go#L129-L130
Added lines #L129 - L130 were not covered by tests
[warning] 133-134: x/crosschain/types/legacy_msgs.go#L133-L134
Added lines #L133 - L134 were not covered by tests
[warning] 137-140: x/crosschain/types/legacy_msgs.go#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-142: x/crosschain/types/legacy_msgs.go#L142
Added line #L142 was not covered by tests
[warning] 145-147: x/crosschain/types/legacy_msgs.go#L145-L147
Added lines #L145 - L147 were not covered by tests
[warning] 150-151: x/crosschain/types/legacy_msgs.go#L150-L151
Added lines #L150 - L151 were not covered by tests
🔇 Additional comments (6)
docs/spec/crosschain/messages.md (1)
Line range hint
105-106: Approval: New fields added toMsgVoteOutbound.The addition of
value_receivedandstatusfields to theMsgVoteOutboundmessage is consistent with the PR objectives. The field numbering is correct, which should maintain compatibility with existing implementations.🧰 Tools
🪛 Markdownlint
63-63: Column: 1
Hard tabs(MD010, no-hard-tabs)
64-64: Column: 1
Hard tabs(MD010, no-hard-tabs)
65-65: Column: 1
Hard tabs(MD010, no-hard-tabs)
66-66: Column: 1
Hard tabs(MD010, no-hard-tabs)
changelog.md (5)
Line range hint
51-71: Approved: Critical fixes and performance improvements.This changelog for version v12.2.4 introduces several important fixes:
- Improved external chain height validation.
- Enhanced gas price buffering for EIP1559.
- Adjusted authorization for WhitelistERC20.
- Improved handling of pending outbound transactions.
- Optimized Bitcoin keysign scheduling.
- Added compatibility for Goerli BlobTxType transactions.
- Enhanced Mumbai empty block handling.
- Improved Bitcoin gas fee calculation using estimated SegWit transaction size.
- Updated Bitcoin confirmation count usage.
These changes address critical issues and should improve the overall stability and performance of the system.
Line range hint
272-305: Security enhancements and transaction handling improvements - comprehensive testing recommended.Version v11.0.0 introduces several important changes:
Security enhancements:
- Added HSM capability for zetaclient hot key.
- Implemented a new thread in zetaclient to check zeta supply in all connected chains in every block.
Transaction handling improvements:
- Fixed minRelayTxFee issue and added checks for misuse of bitcoin mainnet/testnet addresses.
- Improved handling of deposits for paused zrc20.
- Enhanced EVM outbound transaction inclusion.
Refactoring:
- Consolidated node builds.
- Updated
MsgUpdateContractBytecodeto use code hash instead of contract address.Given the security-critical nature of these changes, it is strongly recommended to:
- Conduct thorough testing of the HSM capability, ensuring it properly secures the zetaclient hot key under various scenarios.
- Verify the accuracy and performance of the new zeta supply checking mechanism across all connected chains.
- Perform comprehensive testing of transaction handling, particularly focusing on bitcoin address validation and outbound transaction inclusion for EVM chains.
To verify the implementation of the new security features, please run the following script:
#!/bin/bash # Description: Verify the implementation of HSM capability and zeta supply checking # Test: Check for the presence of HSM-related code rg --type go 'func.*HSM' # Test: Verify the implementation of zeta supply checking rg --type go -A 15 'func.*checkZetaSupply'
Line range hint
307-392: New features and critical fixes introduced - comprehensive testing recommended.Version v10.1.2 brings several important changes:
New features:
- External stress testing capabilities.
- Ability to set liquidity cap for ZRC20.
- Bitcoin block header and merkle proof functionality.
- TSS funds migration capability.
- Transaction to update an observer.
Critical fixes:
- Improved gas handling and stability pool funding.
- Enhanced authorization checks and system contract interactions.
- Fixed issues with blame index updates and emissions grpc server registration.
Refactoring:
- Modified cross-chain call behavior when depositing to a contract.
- Removed duplicate function and optimized gas stability pool funding.
Given the significance of these changes, it is strongly recommended to:
- Conduct thorough stress testing using the new external stress testing capabilities to ensure system stability under high load.
- Verify the correct implementation and behavior of the ZRC20 liquidity cap setting feature.
- Test the TSS funds migration capability extensively to ensure secure and accurate fund transfers.
- Perform comprehensive testing of the modified gas handling and stability pool funding mechanisms.
To verify the implementation of the new features and critical fixes, please run the following script:
#!/bin/bash # Description: Verify the implementation of stress testing capabilities and ZRC20 liquidity cap setting # Test: Check for the presence of stress testing related code rg --type go 'func.*stressTest' # Test: Verify the implementation of ZRC20 liquidity cap setting rg --type go -A 10 'func.*setLiquidityCap' # Test: Check the implementation of TSS funds migration rg --type go -A 15 'func.*migrateTSSFunds'
Line range hint
128-270: Major version update with breaking changes - careful migration planning required.Version v12.0.0 introduces significant architectural changes:
Breaking changes:
- Relocation of TSS and chain validation queries from
crosschaintoobservermodule.- Unified observer set for all chains.
- Merger of observer params and core params into chain params.
- Modified TSS address retrieval for Bitcoin, now requiring Bitcoin chain ID.
New features:
- Support for stateful precompiled contracts.
- Addition of state variable to track aborted zeta amount.
- Implementation of
snapshotscommands.- Dynamic gas price support on zetachain.
Critical fixes:
- Improved handling of unconfirmed UTXOs.
- Enhanced outbound transaction confirmation and inclusion.
- Fixed issues with pending nonces and chain nonces.
Significant refactoring:
- Reorganization of smoke tests structure.
- Movement of TSS state from crosschain to observer.
- Addition of pagination to queries iterating over large data sets.
Given the extensive nature of these changes, it is crucial to:
- Develop a comprehensive migration plan for existing systems.
- Update all client applications to use the new query endpoints and data structures.
- Conduct thorough testing, particularly focusing on Bitcoin transactions and TSS address handling.
- Review and update any documentation or guides that reference the changed modules or queries.
To verify the implementation of the breaking changes, please run the following script:
#!/bin/bash # Description: Verify the relocation of TSS and chain validation queries # Test: Check for the presence of new query endpoints in the observer module rg --type go 'func.*observer.*PendingNonces' rg --type go 'func.*observer.*ChainNonces' rg --type go 'func.*observer.*GetChainParams' # Test: Verify the implementation of Bitcoin chain ID requirement for TSS address retrieval rg --type go -A 10 'func.*GetTssAddress'
Line range hint
73-126: Significant changes introduced - careful testing recommended.Version v12.1.0 introduces several important changes:
New feature: Modified emission distribution to use fixed block rewards.
Critical fixes:
- Improved handling of EVM transaction receipts.
- Enhanced chain parameters comparison logic.
- Exempted system transactions from minimum gas price checks.
- Fixed issues with keygen status and zetaclient stability.
Refactoring:
- Optimized return statements and simplified code.
- Reorganized zetaclient into subpackages.
- Added EVM fee calculation to TSS migration of EVM chains.
Given the significance of these changes, particularly the modifications to emission distribution and gas handling, it is strongly recommended to conduct thorough testing to ensure system stability and correct behavior under various scenarios.
To verify the impact of the emission distribution changes, please run the following script:
Description
VoteGasPriceback in the messageCloses: #2835
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation